Skip to content

Abort more noisily #13

Merged
merged 1 commit into from
Aug 17, 2022
Merged

Abort more noisily #13

merged 1 commit into from
Aug 17, 2022

Conversation

donald
Copy link
Contributor

@donald donald commented Aug 17, 2022

cmirror may abort on various error conditions. There was a complaint,
that the error mesasge might be overlooked because of the normal
diagnostic output:

buczek@theinternet:~$ cmirror /tmp/a /tmp/b --delete
theinternet.molgen.mpg.de: creating empty ./1
theinternet.molgen.mpg.de: creating empty ./10
theinternet.molgen.mpg.de: creating empty ./2
theinternet.molgen.mpg.de: creating empty ./3
theinternet.molgen.mpg.de: creating empty ./4
theinternet.molgen.mpg.de: creating empty ./5
theinternet.molgen.mpg.de: creating empty ./6
theinternet.molgen.mpg.de: creating empty ./7
theinternet.molgen.mpg.de: creating empty ./8
theinternet.molgen.mpg.de: creating empty ./9
theinternet.molgen.mpg.de: rm -r ./xx
rm: cannot remove './xx/aa': Permission denied
buczek@theinternet:~$

To make this more clear, an additional diagnostic output was requested
to show that cmirror didn't finish its job:

buczek@theinternet:~$ cmirror /tmp/a /tmp/b --delete
theinternet.molgen.mpg.de: creating empty ./1
theinternet.molgen.mpg.de: creating empty ./10
theinternet.molgen.mpg.de: creating empty ./2
theinternet.molgen.mpg.de: creating empty ./3
theinternet.molgen.mpg.de: creating empty ./4
theinternet.molgen.mpg.de: creating empty ./5
theinternet.molgen.mpg.de: creating empty ./6
theinternet.molgen.mpg.de: creating empty ./7
theinternet.molgen.mpg.de: creating empty ./8
theinternet.molgen.mpg.de: creating empty ./9
theinternet.molgen.mpg.de: rm -r ./xx
rm: cannot remove './xx/aa': Permission denied

*** cmirror finished with error ***

buczek@theinternet:~$

Implementation:

  • Add code to die() to optionally show the additional
    message. This can be selected by a global flag

  • Let the master set that flag unless --quiet is selected

  • Add code to die() so that it can be called with die(NULL) to show
    only the optional additional message but no specific error message.

  • Replace _exit(1) with die(NULL) n places where it was used to
    abort the parent process after a child process, which as the duty to
    emit the error messages for its operation, has failed.

@donald donald requested a review from wwwutz August 17, 2022 09:22
@wwwutz
Copy link

wwwutz commented Aug 17, 2022

[insert cat picture] PURRFECT. Thanks! I don't need the \n\n , one last line is enough. Thanks!

@donald
Copy link
Contributor Author

donald commented Aug 17, 2022

I've tried this ("*** ... ***\n" instead of "\n*** ... ***\n\n") and had the impression was still easy to overlook, even with the asterisks. Anyway, I don't need the extra output at all (and regret the chatty default instead), so you decide, please.

@wwwutz
Copy link

wwwutz commented Aug 17, 2022

I'd prefer just one last line: cmirror: aborted
no colors, no spaces, no \n\r whatsoever 8-)

cmirror may abort on various error conditions. There was a complaint,
that the error mesasge might be overlooked because of the normal
diagnostic output:

    buczek@theinternet:~$ cmirror /tmp/a /tmp/b --delete
    theinternet.molgen.mpg.de: creating empty ./1
    theinternet.molgen.mpg.de: creating empty ./10
    theinternet.molgen.mpg.de: creating empty ./2
    theinternet.molgen.mpg.de: creating empty ./3
    theinternet.molgen.mpg.de: creating empty ./4
    theinternet.molgen.mpg.de: creating empty ./5
    theinternet.molgen.mpg.de: creating empty ./6
    theinternet.molgen.mpg.de: creating empty ./7
    theinternet.molgen.mpg.de: creating empty ./8
    theinternet.molgen.mpg.de: creating empty ./9
    theinternet.molgen.mpg.de: rm -r ./xx
    rm: cannot remove './xx/aa': Permission denied
    buczek@theinternet:~$

To make this more clear, an additional diagnostic output was requested
to show that cmirror didn't finish its job:

    buczek@theinternet:~$ cmirror /tmp/a /tmp/b --delete
    theinternet.molgen.mpg.de: creating empty ./1
    theinternet.molgen.mpg.de: creating empty ./10
    theinternet.molgen.mpg.de: creating empty ./2
    theinternet.molgen.mpg.de: creating empty ./3
    theinternet.molgen.mpg.de: creating empty ./4
    theinternet.molgen.mpg.de: creating empty ./5
    theinternet.molgen.mpg.de: creating empty ./6
    theinternet.molgen.mpg.de: creating empty ./7
    theinternet.molgen.mpg.de: creating empty ./8
    theinternet.molgen.mpg.de: creating empty ./9
    theinternet.molgen.mpg.de: rm -r ./xx
    rm: cannot remove './xx/aa': Permission denied
    cmirror aborted
    buczek@theinternet:~$

Implementation:

- Add code to `die()` to optionally show the additional
  message. This can be selected by a global flag

- Let the master set that flag unless `--quiet` is selected

- Add code to `die()` so that it can be called with `die(NULL)` to show
  only the optional additional message but no specific error message.

- Replace `_exit(1)` with `die(NULL)` n places where it was used to
  abort the parent process after a child process, which as the duty to
  emit the error messages for its operation, has failed.
Copy link

@wwwutz wwwutz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@donald donald merged commit 1c765e9 into main Aug 17, 2022
Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants